-
Notifications
You must be signed in to change notification settings - Fork 8
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Ip6 scoped linklocal #113
base: main
Are you sure you want to change the base?
Ip6 scoped linklocal #113
Conversation
We may have a similar linklocal next hop (same ip, same mac) on 2 different interfaces. In order to support scoped linklocal addresses, we encode the interface id in the second nibble of the ipv6 address: "fe80::209:c000:17e:f6b on interface id 1" becomes "fe80:100::209:c000:17e:f6b". Signed-off-by: Christophe Fontaine <cfontain@redhat.com>
Do not expose the internal encoding of the interface in the link local address. Signed-off-by: Christophe Fontaine <cfontain@redhat.com>
I'll update the API to allow the client to specify the interface. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hi chris, thanks for working on this. I think we should read carefully the bsd stack and get inspiration from where they scope and unscope the addresses.
Here are a couple of pointers which may be good to check:
static int ip6_mcast_addr_add(struct iface *iface, const struct rte_ipv6_addr *ip) { | ||
static int ip6_mcast_addr_add(struct iface *iface, const struct rte_ipv6_addr *ipv6) { | ||
struct hoplist *maddrs = &iface_mcast_addrs[iface->id]; | ||
struct nexthop *nh = NULL; | ||
struct rte_ipv6_addr ip = *ipv6; | ||
unsigned i; | ||
|
||
LOG(INFO, "%s: joining multicast group " IP6_F, iface->name, ip); | ||
LOG(INFO, "%s: joining multicast group " IP6_F, iface->name, &ip); | ||
|
||
for (i = 0; i < maddrs->count; i++) | ||
if (rte_ipv6_addr_eq(&maddrs->nh[i]->ipv6, ip)) | ||
if (rte_ipv6_addr_eq(&maddrs->nh[i]->ipv6, &ip)) | ||
return errno_set(EEXIST); | ||
|
||
if (i == ARRAY_DIM(maddrs->nh)) | ||
return errno_set(ENOSPC); | ||
|
||
if ((nh = ip6_nexthop_lookup(iface->vrf_id, ip)) == NULL) { | ||
if ((nh = ip6_nexthop_new(iface->vrf_id, GR_IFACE_ID_UNDEF, ip)) == NULL) | ||
if ((nh = ip6_nexthop_lookup(iface->vrf_id, &ip)) == NULL) { | ||
if ((nh = ip6_nexthop_new(iface->vrf_id, GR_IFACE_ID_UNDEF, &ip)) == NULL) | ||
return errno_set(-errno); | ||
rte_ether_mcast_from_ipv6(&nh->lladdr, ip); | ||
rte_ether_mcast_from_ipv6(&nh->lladdr, &ip); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think I missed something. Why are you introducing a new local ip variable to use it instead of the const parameter?
struct hoplist *addrs; | ||
unsigned addr_index; | ||
struct rte_ipv6_addr *ip = (struct rte_ipv6_addr *)ipv6; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
if we need to change the signature of the function we can discuss it, no worries. But please no hacks like this.
|
||
if ((ret = iface6_addr_add(iface, &req->addr.addr.ip, req->addr.addr.prefixlen)) < 0) | ||
ip6_addr_linklocal_scope(&ip, req->addr.iface_id); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
you are adding the iface id scope twice: here and in iface6_addr_add.
scoped = req->addr.addr.ip; | ||
ip6_addr_linklocal_scope(&scoped, req->addr.iface_id); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think it is safe to scope the address present in the request. no need for a local variable.
@@ -40,4 +40,18 @@ struct hoplist *ip6_addr_get_all(uint16_t iface_id); | |||
// determine if the given interface is member of the provided multicast address group | |||
struct nexthop *ip6_mcast_get_member(uint16_t iface_id, const struct rte_ipv6_addr *mcast); | |||
|
|||
static inline int ip6_addr_linklocal_scope(struct rte_ipv6_addr *ip, uint16_t iface_id) { | |||
if (!rte_ipv6_addr_is_linklocal(ip)) | |||
return errno_set(EFAULT); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
EINVAL would be more appropriate.
@@ -40,4 +40,18 @@ struct hoplist *ip6_addr_get_all(uint16_t iface_id); | |||
// determine if the given interface is member of the provided multicast address group | |||
struct nexthop *ip6_mcast_get_member(uint16_t iface_id, const struct rte_ipv6_addr *mcast); | |||
|
|||
static inline int ip6_addr_linklocal_scope(struct rte_ipv6_addr *ip, uint16_t iface_id) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I am not fond of the function name but I don't have great alternatives to suggest either.
static inline int ip6_addr_linklocal_scope(struct rte_ipv6_addr *ip, uint16_t iface_id) { | ||
if (!rte_ipv6_addr_is_linklocal(ip)) | ||
return errno_set(EFAULT); | ||
((uint16_t *)ip->a)[1] = iface_id; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It would be best to change this to network order to ensure consistent behavior across architectures.
rte_be16_t scope_id = htons(iface_id);
// rte_ipv6_addr is aligned to 1, avoid alignment issues with casting to uint16_t
ip->a[2] = 0xff & (scope_id >> 8);
ip->a[3] = 0xff & scope_id;
static inline int ip6_addr_linklocal_unscope(struct rte_ipv6_addr *ip) { | ||
if (!rte_ipv6_addr_is_linklocal(ip)) | ||
return errno_set(EFAULT); | ||
((uint16_t *)ip->a)[1] = 0; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
// rte_ipv6_addr is aligned to 1, avoid alignment issues with casting to uint16_t
ip->a[2] = 0;
ip->a[3] = 0;
First try to implement scoped link local addresses.
Still has some issues:
This link local address doesn't exist in the FIB, but fe80:100::4e77:6dff:fe7d:1cc7 and fe80:300::4e77:6dff:fe7d:1cc7 are present internally.